Let's not change the function signature here and in the other builders

Could you be more specific about what you are looking for? Checking the code base, I see 25 calls to this constructor, and in 10/25 of the cases, I need to pass file_format_options to maintain the needed settings. I guess I could eliminate the 15 cases where I pass None by splitting this into two named constructors...

You need to use DeltaTableConfig for this:

OK. I think you are suggesting that I add file_format_options to the config member of DeltaTable.
In fact, that was the approach I tried initially but I had to back that out and add this as a direct member for the following reasons:

1. The file format options don't really seem to fit properly into DeltaTableConfig since these options seem to be more about managing the logfile.
2. We need to preserve the formatting options when going from DeltaTable to DeltaOps and back. Right now, the config gets lost when new_with_state is called. See below where the config gets reset to default:
   

   delta-rs/crates/core/src/table/mod.rs

   Line 128 in 18f949e

   pub(crate) fn new_with_state(log_store: LogStoreRef, state: DeltaTableState) -> Self {

   
   We need to preserve these settings throughout any chained operations. This means we still need an extra parameter in new_with_state to preserve any existings config. Also, I felt it was cleaner to create a new entry and directly set and pass it. Possibly we could move this into DeltaTableConfig but I think this may be more of a hindrance than a help.

@adamreeve It was a couple of months ago that we moved this, do you remember anything else from our discussion? See commit below:
666f0ba

1. Correct but they are related to loading the table, which file formats (encryption) should belong as well
2. DeltaTableConfig tags along with the snapshot:

   delta-rs/crates/core/src/table/state.rs

   Lines 73 to 76 in a61ac16

   	/// Get the table config which is loaded with of the snapshot
   	pub fn load_config(&self) -> &DeltaTableConfig {
   	self.snapshot.load_config()
   	}

so that shouldn't be an issue to allow it to stay there when you do new_with_state

OK. I think it makes sense to move this here. I will investigate this week. The main issue is whether I have to support serialization out of the gate to make this work / at what points config gets reconstructed from serialized properties. The reason why is that serialization of the FileFormatOptions will be a bit tricky because:

1. I'm not sure if TableOptions fully supports serialization.
2. For direct encryption and decryption properties, we will need to modify the serialization to make sure that passwords don't get serialized.

I think it would be better if this pushed into the LoadConfig and not passed through each function

Can you elaborate a bit here? There are a lot of these different execute functions (one for every option) and I agree that it would be nice if they had a common configuration structure rather than the somewhat long list of arguments they take. That would probably be independent of this PR. For now, we have just added an additional argument to pass the needed settings through to execution.

See ping above, it needs to be DeltaTableConfig

@ion-elgreco We could use some feedback on the best way to set the config for an existing table. In this design, we wanted to:

1. Be able to set the config at runtime, not just at table construction. This is important for fields like passwords, where we will not want them to be serialized, or other options that we may want to change at runtime.
2. Make the user interface easy to use.
   With this design, setting the config for any operation looks like: (from crates/deltalake/examples/basic_operations_encryption.rs)

    let ops = DeltaOps::try_from_uri(url).await?;
    let ops = ops
        .with_file_format_options(file_format_options.clone())
        .await?;

We also considered a design where you could only set this via DeltaTableBuilder but I wanted to make this feature easy to use. See e.g. the following diff where this function is removed.

adamreeve@ec40881

@adamreeve

I've just pushed a fixed commit that handles when the load fails if the table isn't created yet: adamreeve@0d551a5

This matches the behaviour of DeltaOps::try_from_uri. Maybe there could be a DeltaOps::try_from_table or TryFrom<DeltaTable> for DeltaOps implementation to handle that scenario.

I am having hard time understanding this usecase. Why wouldn't it be enough to update the DeltaTableConfig, I would assume without setting the encryption you couldn't load the table at all

This is not needed anymore, since you can access the snapshot.load_config() to access the file_format_options

Same here it's already available in the snapshot. so we can defer at latest stage to grab these from the load_config

I prefer we Impl Into here between these two structs.

I suggest we remove this here. We can move all this logic into:

DeltaScanConfigBuilder.build(), there we can introspect the TableLoadConfig and set the parquetOptions on the DeltaScanConfig

This can be removed since this will happen inside the DeltaScanBuilder with my suggestion above

Same here, not needed anymore, since we can pass it through the DeltaScanConfig

Why can this return None as well?

56.2.0 has landed so this can be removed

Do we really need a function for this 🤷

unwrap_or_default is more idiomatic

same applies here, I dont think we need a function for just a map()